Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sysroot: Add concept of deployment "pinning" 📌 #1464

Closed
wants to merge 3 commits into from

Conversation

cgwalters
Copy link
Member

See commits.

Also has a prep commit for transient state which I plan to use for "pending" state AKA #545

@cgwalters cgwalters changed the title sysroot: Add concept of deployment "pinning" pushpin sysroot: Add concept of deployment "pinning" 📌 Feb 23, 2018
@dustymabe
Copy link
Contributor

any chance this could be extended to support the use case described in #1460 (comment) and coreos/rpm-ostree#577 ?

@cgwalters
Copy link
Member Author

any chance this could be extended to support the use case described in #1460 (comment) and coreos/rpm-ostree#577 ?

It's not clear what you want there - something like a config file where you can change 2 to 3 or 5 etc? The semantics of that would seem...strange to me. It could be implemented on top of this by pinning a number anyways.

@dustymabe
Copy link
Contributor

It's not clear what you want there - something like a config file where you can change 2 to 3 or 5 etc?

yes.

The semantics of that would seem...strange to me.

To me this is basically the same thing as saying "keep around X number of snapshots". For example on my desktop system I have btrfs snapshots and I essentially keep them all; I have lots of disk so why not? It would be great if I could configure ostree/rpm-ostree to do something similar so I can start moving my systems over to FAW.

It could be implemented on top of this by pinning a number anyways.

Not sure what that means exactly. So instead of a deployment the user could pin X where X represents the number of deployments to keep?

@cgwalters
Copy link
Member Author

For example on my desktop system I have btrfs snapshots and I essentially keep them all; I have lots of disk so why not? It would be great if I could configure ostree/rpm-ostree to do something similar so I can start moving my systems over to FAW.

But do you e.g. snapshot /home too? It's not really the same thing; ostree isn't a backup system.

The way I think of it is; keeping lots of snapshots locally doesn't seem to make sense because you can re-download the from the Internet on-demand. (And yes, that's an experience we need to consistently enable with rpm-ostree and packages).

But if you still want it, I'm not opposed to adding more policies, but it seems orthogonal to this PR.

@dustymabe
Copy link
Contributor

But do you e.g. snapshot /home too? It's not really the same thing; ostree isn't a backup system.

In my case I do (i actually snapshot the entire filesystem), but I get that ostree isn't going to be exactly what I currently have. I've actually thought about writing a snapper plugin for ostree, like the dnf one, that will call snapper and ask it to make snapshots (btrfs, or thin LV snapshots) automatically on configured filesystems whenever an rpm-ostree operation happens. Haven't really got far on that idea, just a thought so far.

The way I think of it is; keeping lots of snapshots locally doesn't seem to make sense because you can re-download the from the Internet on-demand. (And yes, that's an experience we need to consistently enable with rpm-ostree and packages).

That is true, but for some reason I lean towards wanting that functionality. if I rebase from f27 to rawhide AND then layer one package my f27 deployment is gone. I'd like it to be a little easier to keep around old deployments than that. The pinning helps, but only works if you proactively think about it.

But if you still want it, I'm not opposed to adding more policies, but it seems orthogonal to this PR.

Thank you! I guess it's orthogonal, seemed like a slight variation of the same thing to me.

@cgwalters
Copy link
Member Author

if I rebase from f27 to rawhide AND then layer one package my f27 deployment is gone. I'd like it to be a little easier to keep around old deployments than that. The pinning helps, but only works if you proactively think about it.

I think we'll edit the "rebase instructions" page to include --pin.

* for e.g. older versions of libostree unaware of pinning to GC the deployment.
*
* This function does nothing and returns successfully if the deployment
* is already in the desired pinning state.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since: ?

assert_n_pinned 2
os_repository_new_commit
${CMD_PREFIX} ostree admin upgrade --os=testos
assert_n_pinned 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let's explicitly also check that the pinned deployments are still there? (I.e. just grep for checksums? Or maybe just checking that the deployment count went up to three.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah...we can do a lot of that but it requires reinventing a lot of the things that were in rpm-ostree. Deduplicating that is going to be up at some point.

We'll get test coverage the other way though when we wrap this in rpm-ostree and do some tests there.


/* ATTENTION:
* Please remember to update the bash-completion script (bash/ostree) and
* man page (man/ostree-admin-undeploy.xml) when changing the option list.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment. Also doesn't look like we added bash completions for this, right?
Let's at least change it to a TODO?

if (is_pinned == current_pin)
return TRUE;

g_autoptr(OstreeDeployment) deployment_clone = ostree_deployment_clone (deployment);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused here. Why do we need ostree_deployment_clone? Can't we just ostree_deployment_get_origin and modify that? (Actually, shouldn't we do that so that the sysroot deployment array is up to date?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the sysroot array should be immutable. That actually reveals a bug, we need to bump the mtime so e.g. rpm-ostree will do a reload.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh got it, that makes sense.

This ensures that e.g. `rpm-ostreed` will get notified of the changes.
The `origin/unlocked` and `origin/override-commit` keys are examples of state
that's really transient; we don't want to maintain them across upgrades. Right
now there are bits for this in both `ostree admin upgrade` as well as in
rpm-ostree.

This new API will slightly clean up both cases, but it's really prep for adding
a concept of deployment "pinning" that will live in the new
`libostree-transient` group.
@cgwalters
Copy link
Member Author

Now with another prep commit, and squashed a few fixups ⬆️

Example user story: Jane rebases her OS to a new major version N, and wants to
keep around N-1 even after a few upgrades for a while so she can easily roll
back. I plan to add `rpm-ostree rebase --pin` to opt-in to this for example.

Builds on the new `libostree-transient` group to store pinning state there.

Closes: ostreedev#1460
@@ -399,6 +399,37 @@ _ostree_admin_os_init() {
return 0
}

_ostree_admin_pin() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, we also need a pin entry in _ostree_admin().

@@ -399,6 +399,37 @@ _ostree_admin_os_init() {
return 0
}

_ostree_admin_pin() {
local boolean_options="
$main_boolean_options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--unpin ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't grasp bash completion, I don't care too much about it myself at least for ostree admin. Let's not try to polish it for now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There has to be someone out there who has implemented actually generating this stuff from the cli --help, as is it's an enormous maintenance pain doubled by the fact that we need to wrap it in rpm-ostree at least too.

@jlebon
Copy link
Member

jlebon commented Feb 26, 2018

Sure, that's fine with me.
@rh-atomic-bot r+ 4374dcf

@rh-atomic-bot
Copy link

⚡ Test exempted: pull fully rebased and already tested.

rh-atomic-bot pushed a commit that referenced this pull request Feb 26, 2018
The `origin/unlocked` and `origin/override-commit` keys are examples of state
that's really transient; we don't want to maintain them across upgrades. Right
now there are bits for this in both `ostree admin upgrade` as well as in
rpm-ostree.

This new API will slightly clean up both cases, but it's really prep for adding
a concept of deployment "pinning" that will live in the new
`libostree-transient` group.

Closes: #1464
Approved by: jlebon
rh-atomic-bot pushed a commit that referenced this pull request Feb 26, 2018
Example user story: Jane rebases her OS to a new major version N, and wants to
keep around N-1 even after a few upgrades for a while so she can easily roll
back. I plan to add `rpm-ostree rebase --pin` to opt-in to this for example.

Builds on the new `libostree-transient` group to store pinning state there.

Closes: #1460

Closes: #1464
Approved by: jlebon
cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Mar 8, 2018
Implemented in libostree in ostreedev/ostree#1464
Let's display it - wrapping the command will come later.

I also just noticed `rpmostree_syscore_filter_deployments()` at least is
going to have to learn about pinning; will need to improve the test suite
around this too.
rh-atomic-bot pushed a commit to coreos/rpm-ostree that referenced this pull request Mar 8, 2018
Implemented in libostree in ostreedev/ostree#1464
Let's display it - wrapping the command will come later.

I also just noticed `rpmostree_syscore_filter_deployments()` at least is
going to have to learn about pinning; will need to improve the test suite
around this too.

Closes: #1292
Approved by: jlebon
cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Mar 22, 2018
Followup to: ostreedev/ostree#1464

Ideally, we'd delegate more logic around these things to libostree, but we're
not there yet.

This means e.g. `rpm-ostree cleanup -r` for a pinned rollback will just silently
skip it.  It'd be nicer to emit an error probably, but that'd be quite a bit
more work.

Closes: coreos#1293
rh-atomic-bot pushed a commit to coreos/rpm-ostree that referenced this pull request Mar 23, 2018
Followup to: ostreedev/ostree#1464

Ideally, we'd delegate more logic around these things to libostree, but we're
not there yet.

This means e.g. `rpm-ostree cleanup -r` for a pinned rollback will just silently
skip it.  It'd be nicer to emit an error probably, but that'd be quite a bit
more work.

Closes: #1293

Closes: #1309
Approved by: jlebon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants